JSON.generate: call to_json on String subclasses #668
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix: ruby#667 This is yet another behavior on which the various implementations differed, but the C implementation used to call `to_json` on String subclasses used as keys. This was optimized out in e125072 but there is an Active Support test case for it, so it's best to make all 3 implementation respect this behavior.
|
This seems a bit unfortunate performance-wise. |
|
I think the performance cost is very minor, just an extra pointer comparison on the class, that's tagged as unlikely, so probably predicted out. I didn't see much change on the benchmarks. |
|
In lib/json/pure/generator.rb |
|
My assumption is the pure generator is only really used with Truffle or in context where perf was disregarded. Have you measured the perf degradation on Truffle, is it really substantial? I'd expect Truffle to be able to optimize most of that out. |
|
I'll check it out later. |
|
Before this PR (6d3b3ac): After this PR (96397cf): |
|
I tried to undo the changes incrementally and the main slowdown seems from using: klass = obj.class
if klass == Hash
elsif klass == Array
elsif klass == String
...(I also tried to reverse that == check but it didn't seem to change perf much, the reasoning is vs case obj
when Hash
when Array
when String
...I wonder if maybe it's simply |
|
And how fast is the C implementation on Truffle? Because on my machine: So maybe Truffle should just revert back to use the C extension? |
eregon
added a commit
to eregon/json
that referenced
this pull request
Nov 1, 2024
* if/elsif comparing `obj.class` is significantly slower: ruby#668 (comment) * The only case where an exact class check is needed so far is for String (ruby#667). * Before: $ ruby -Ilib:ext benchmark/standalone.rb dump pure JSON::Pure::Generator truffleruby 24.2.0-dev-07b978e4, like ruby 3.2.4, Oracle GraalVM JVM [x86_64-linux] Calculating ------------------------------------- JSON.dump(obj) 6.426k (± 5.9%) i/s (155.62 μs/i) - 32.395k in 5.064479s JSON.dump(obj) 6.380k (± 7.4%) i/s (156.73 μs/i) - 31.806k in 5.021304s JSON.dump(obj) 6.276k (±10.5%) i/s (159.33 μs/i) - 31.217k in 5.060762s JSON.dump(obj) 6.450k (± 7.0%) i/s (155.05 μs/i) - 32.395k in 5.059538s JSON.dump(obj) 6.413k (± 6.2%) i/s (155.93 μs/i) - 32.395k in 5.081573s * After: $ ruby -Ilib:ext benchmark/standalone.rb dump pure JSON::Pure::Generator truffleruby 24.2.0-dev-07b978e4, like ruby 3.2.4, Oracle GraalVM JVM [x86_64-linux] Calculating ------------------------------------- JSON.dump(obj) 8.237k (± 5.0%) i/s (121.41 μs/i) - 41.600k in 5.069507s JSON.dump(obj) 8.179k (± 5.1%) i/s (122.26 μs/i) - 40.768k in 5.002035s JSON.dump(obj) 8.147k (± 7.9%) i/s (122.74 μs/i) - 40.768k in 5.044840s JSON.dump(obj) 8.137k (± 6.9%) i/s (122.90 μs/i) - 40.768k in 5.048690s JSON.dump(obj) 8.112k (±10.2%) i/s (123.27 μs/i) - 39.936k in 5.023502s
|
Unfortunately the C extension is still much slower, needs some investigation & profiling: Compared to after #674: To have a baseline: |
eregon
added a commit
to eregon/json
that referenced
this pull request
Nov 4, 2024
* if/elsif comparing `obj.class` is significantly slower: ruby#668 (comment) * The only case where an exact class check is needed so far is for String (ruby#667). * Before: $ ruby -Ilib:ext benchmark/standalone.rb dump pure JSON::Pure::Generator truffleruby 24.2.0-dev-07b978e4, like ruby 3.2.4, Oracle GraalVM JVM [x86_64-linux] Calculating ------------------------------------- JSON.dump(obj) 6.426k (± 5.9%) i/s (155.62 μs/i) - 32.395k in 5.064479s JSON.dump(obj) 6.380k (± 7.4%) i/s (156.73 μs/i) - 31.806k in 5.021304s JSON.dump(obj) 6.276k (±10.5%) i/s (159.33 μs/i) - 31.217k in 5.060762s JSON.dump(obj) 6.450k (± 7.0%) i/s (155.05 μs/i) - 32.395k in 5.059538s JSON.dump(obj) 6.413k (± 6.2%) i/s (155.93 μs/i) - 32.395k in 5.081573s * After: $ ruby -Ilib:ext benchmark/standalone.rb dump pure JSON::Pure::Generator truffleruby 24.2.0-dev-07b978e4, like ruby 3.2.4, Oracle GraalVM JVM [x86_64-linux] Calculating ------------------------------------- JSON.dump(obj) 8.237k (± 5.0%) i/s (121.41 μs/i) - 41.600k in 5.069507s JSON.dump(obj) 8.179k (± 5.1%) i/s (122.26 μs/i) - 40.768k in 5.002035s JSON.dump(obj) 8.147k (± 7.9%) i/s (122.74 μs/i) - 40.768k in 5.044840s JSON.dump(obj) 8.137k (± 6.9%) i/s (122.90 μs/i) - 40.768k in 5.048690s JSON.dump(obj) 8.112k (±10.2%) i/s (123.27 μs/i) - 39.936k in 5.023502s
casperisfine
pushed a commit
to Shopify/ruby
that referenced
this pull request
Nov 5, 2024
…es subclasses Ref: ruby/json#674 Ref: ruby/json#668 The behavior on such case it quite unclear, the goal here is to figure out whatever was the behavior on Cext version of `json 2.7.0` and get all implementations to converge. We can then decide to make them all behave differently if we so wish. ruby/json@614921dcef
casperisfine
pushed a commit
to Shopify/ruby
that referenced
this pull request
Nov 5, 2024
…es subclasses Ref: ruby/json#674 Ref: ruby/json#668 The behavior on such case it quite unclear, the goal here is to figure out whatever was the behavior on Cext version of `json 2.7.0` and get all implementations to converge. We can then decide to make them all behave differently if we so wish. ruby/json@614921dcef
byroot
added a commit
to ruby/ruby
that referenced
this pull request
Nov 5, 2024
…es subclasses Ref: ruby/json#674 Ref: ruby/json#668 The behavior on such case it quite unclear, the goal here is to figure out whatever was the behavior on Cext version of `json 2.7.0` and get all implementations to converge. We can then decide to make them all behave differently if we so wish. ruby/json@614921dcef
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix: #667
This is yet another behavior on which the various implementations differed, but the C implementation used to call
to_jsonon String subclasses used as keys.This was optimized out in e125072 but there is an Active Support test case for it, so it's best to make all 3 implementation respect this behavior.
FYI: @mtasaka